feat(helm): migrate away from bitnami helm chart#2569
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaces Bitnami Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
pepol
force-pushed
the
peter/eng-8854-keycloakx-chart
branch
from
4063dcc to
fb236c4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@helm/cosmo/README.md`:
- Line 181: The Markdown table row for keycloak.extraEnv contains unescaped pipe
characters in the default multiline string (references: keycloak.extraEnv,
KC_BOOTSTRAP_ADMIN_PASSWORD, .Values.global.keycloak.adminPassword) which breaks
the table; fix it by escaping each pipe inside the string (e.g., replace "|"
with "\|" or use "|") so the table parser treats them as literal characters
rather than column separators, ensuring the value string remains unchanged
semantically but safe for Markdown rendering.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
helm/cosmo/values.yaml (1)
359-362: Production consideration:start-devmode should not be used in production.The
start-devargument disables TLS and enables dev-mode features. While the comment at line 360 notes "Production requires TLS", this configuration as default values could lead to accidental production deployments without TLS.Consider either:
- Adding a more prominent warning in the values file
- Providing a separate production-ready configuration example
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@helm/cosmo/values.yaml` around lines 359 - 362, The values file currently includes the 'start-dev' entry under args which disables TLS and should never be the default for production; remove 'start-dev' from the default args list, add a prominent WARNING comment above the args block calling out that 'start-dev' must not be used in production, and add a separate commented "production example" values snippet (without 'start-dev' and showing the TLS-related flags/configuration) so operators have a safe, copy-paste production configuration; reference and update the args array and the 'start-dev' token in the helm values to implement this.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@helm/cosmo/values.yaml`:
- Around line 359-362: The values file currently includes the 'start-dev' entry
under args which disables TLS and should never be the default for production;
remove 'start-dev' from the default args list, add a prominent WARNING comment
above the args block calling out that 'start-dev' must not be used in
production, and add a separate commented "production example" values snippet
(without 'start-dev' and showing the TLS-related flags/configuration) so
operators have a safe, copy-paste production configuration; reference and update
the args array and the 'start-dev' token in the helm values to implement this.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
helm/cosmo/Chart.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
helm/cosmo/CHART.mdhelm/cosmo/Chart.yamlhelm/cosmo/README.mdhelm/cosmo/charts/keycloak-22.0.0.tgzhelm/cosmo/charts/keycloakx-7.1.8.tgzhelm/cosmo/values.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- helm/cosmo/Chart.yaml
pepol
force-pushed
the
peter/eng-8854-keycloakx-chart
branch
from
cb408a6 to
c0d002a
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@helm/cosmo/README.md`:
- Line 181: The README at helm/cosmo/README.md has drift (the rendered table row
for keycloak.extraEnv doesn't match generated output) and CI fails; regenerate
the chart documentation with the exact tool/version used by CI (the chart doc
generator that produces the keycloak.extraEnv table row) and commit the
regenerated README so the rendered table (the keycloak.extraEnv row) matches
exactly what CI expects; ensure you run the same command/version as CI, verify
the keycloak.extraEnv row now matches, and commit the updated README.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@helm/cosmo/values.yaml`:
- Around line 359-362: The Helm values currently set Keycloak to start in dev
mode by default (args contains 'start-dev'); change the default startup arg in
the args array from 'start-dev' to 'start' so production uses the proper kc.sh
start behavior, leaving 'start-dev' available only as an explicit override for
local/dev deployments (update any related docs or comments to note that
'start-dev' is for local use only).
- Around line 395-399: The extraEnv entries for KC_BOOTSTRAP_ADMIN_USERNAME and
KC_BOOTSTRAP_ADMIN_PASSWORD render raw template values which can break YAML when
the credentials contain special characters; update the value expressions for
.Values.global.keycloak.adminUser and .Values.global.keycloak.adminPassword to
produce quoted/escaped strings (use the Helm quote function or ensure the
template output is wrapped in double quotes) so the produced manifest remains
valid even with colons, hashes, or quotes in the credentials.
c0d002a to
3dc8902
Compare
pepol
force-pushed
the
peter/eng-8854-keycloakx-chart
branch
from
3dc8902 to
0b213d3
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@helm/cosmo/CHART.md`:
- Line 27: Update the Keycloak Helm repo URL in CHART.md: replace the incorrect
repository string "https://codecentric.github.io/helm-chart" in the table row
containing "keycloakx" with the correct
"https://codecentric.github.io/helm-charts" so the entry remains "|
https://codecentric.github.io/helm-charts | keycloakx | ^7.1.8 |".
In `@helm/cosmo/values.yaml`:
- Line 64: The ingress backend is routing to the wrong Keycloak service name;
update the ingress template so the backend service name matches the actual
Keycloak service created (cosmo-keycloak-http) or change the value used by the
include. Specifically, reconcile the mismatch between the values key apiUrl
(apiUrl: 'http://cosmo-keycloak-http:8080') and the ingress backend reference
({{ include "keycloak.fullname" .}) by modifying the ingress backend to
reference the http service (the fullname for the Keycloak HTTP service) or
adjust the keycloak chart include used in the ingress template so it resolves to
the http service name (ensure the template uses the Keycloak HTTP service
fullname rather than the nonexistent base service).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cad90f2a-f629-4b3c-8ddc-1a0d91b58489
⛔ Files ignored due to path filters (1)
helm/cosmo/Chart.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
helm/cosmo/CHART.mdhelm/cosmo/Chart.yamlhelm/cosmo/README.mdhelm/cosmo/charts/keycloak-22.0.0.tgzhelm/cosmo/charts/keycloakx-7.1.8.tgzhelm/cosmo/values.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- helm/cosmo/Chart.yaml
pepol
force-pushed
the
peter/eng-8854-keycloakx-chart
branch
from
0b213d3 to
345ad2e
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
helm/cosmo/values.yaml (1)
359-362: Defaultstart-devmode is appropriate for this dev-focused chart but document production guidance.The
start-devargument is suitable given the chart's stated purpose (lines 344-349 explicitly state these charts are for development/testing, not production). The inline comment on line 360 acknowledges TLS requirements for production.Consider adding a note in the README or DEV.md explaining that production deployments should override
argsto usestartwith proper TLS/hostname configuration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@helm/cosmo/values.yaml` around lines 359 - 362, The chart currently defaults the container args list (args: including 'start-dev', '--import-realm', '--optimized') for development; add a short note to the chart README or DEV.md explaining that production deployments must override these Helm values (the args key) to use the non-dev command (replace 'start-dev' with 'start'), and must supply proper TLS certificate, hostname, and related configuration (e.g., ingress/host, TLS secret) via Helm values or values.yaml overrides; mention the exact values key to change (args) and suggest using helm --set or a production values file to perform the override.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@helm/cosmo/values.yaml`:
- Around line 359-362: The chart currently defaults the container args list
(args: including 'start-dev', '--import-realm', '--optimized') for development;
add a short note to the chart README or DEV.md explaining that production
deployments must override these Helm values (the args key) to use the non-dev
command (replace 'start-dev' with 'start'), and must supply proper TLS
certificate, hostname, and related configuration (e.g., ingress/host, TLS
secret) via Helm values or values.yaml overrides; mention the exact values key
to change (args) and suggest using helm --set or a production values file to
perform the override.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fcb5636d-8564-4e74-bf8a-e3df8b2c8f99
⛔ Files ignored due to path filters (1)
helm/cosmo/Chart.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
helm/cosmo/CHART.mdhelm/cosmo/Chart.yamlhelm/cosmo/README.mdhelm/cosmo/charts/keycloak-22.0.0.tgzhelm/cosmo/charts/keycloakx-7.1.8.tgzhelm/cosmo/templates/ingress.yamlhelm/cosmo/values.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- helm/cosmo/CHART.md
pepol
force-pushed
the
peter/eng-8854-keycloakx-chart
branch
3 times, most recently
from
b251ae4 to
03e2a93
Compare
pepol
force-pushed
the
peter/eng-8854-keycloakx-chart
branch
from
03e2a93 to
f8e2a28
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Checklist